Add Tavily web search integration and Telegram typing indicator#3
Merged
Conversation
- Add web_search provider abstraction with Tavily (default) and Perplexity backends - Auto-detect provider from env vars (TAVILY_API_KEY > PERPLEXITY_API_KEY), with explicit override via WEB_SEARCH_PROVIDER - Add tavily-search as external skill with embedded script and registry entry - Add `forge skills add` command for vendoring registry skills into projects - Update init wizard with provider selection phase when web_search is chosen - Send Telegram "typing" chat action while LLM processes requests
- Add CLAUDE.md with pre-commit gofmt and golangci-lint instructions - Wire validateTavilyKey into the init wizard tools step - Fix gofmt formatting in skills.go and tools_step.go - Rename ValidatePerplexityFunc to ValidateWebSearchKeyFunc for both providers
…alidation - Egress step now only shows the selected web_search provider domain (Tavily or Perplexity) instead of both - Skills step prompts for required_env, one_of_env, and optional_env when skills are selected, blocking if required/one_of is not provided - Web search tool step validates Tavily/Perplexity API keys with async validation (same pattern as provider step) - DeriveEgressFunc now receives envVars to filter provider-specific domains - Updated tests to match new provider-specific behavior
Tools line now displays "web_search [tavily]" or "web_search [perplexity]" to indicate which provider was selected.
The review step reads from WizardContext directly, not the tools step Summary(). Updated Prepare() to annotate web_search with the provider from ctx.EnvVars.
Init() was not resetting phase, currentPrompt, envPrompts, or envValues. When the user navigated back from a later step, the stale skillsEnvPhase with an exhausted currentPrompt caused an index out of range panic.
Skills step now checks env vars collected by earlier wizard steps (via Prepare/knownEnvVars) so it won't re-prompt for keys like OPENAI_API_KEY already provided as the model provider. buildEnvVars() uses a written map to skip duplicate keys when writing the .env file.
Provider step Apply() now stores the API key in ctx.EnvVars (e.g. OPENAI_API_KEY) so the skills step's Prepare() can detect it via knownEnvVars and skip the one_of group entirely.
MultiSelect and SingleSelect Init() methods were no-ops that didn't reset the done flag. After navigating back, done remained true causing Update() to short-circuit and treat all keys (including space) as no-ops while the step immediately re-completed.
Three issues fixed: - Wizard now handles esc globally as quit (matching kbd hints) - Removed redundant esc→back mapping from review step that caused the review↔egress loop - EgressDisplay.Init() now resets done flag so back-navigation doesn't immediately re-advance
naveen-kurra
pushed a commit
to naveen-kurra/forge
that referenced
this pull request
May 23, 2026
…ializ#3) Reviewer flagged: forge-core/auth/middleware.go silently returned a no-op wrapper when opts.Chain == nil. A misconfigured runner that forgot to wire a chain would silently serve unauthenticated requests in production. High severity — the open-prod-endpoint failure mode is the highest-impact misconfiguration in the auth subsystem. Fix: - New MiddlewareOptions.AllowAnonymous explicit opt-in (default false). - Middleware() panics at construction when Chain == nil && !AllowAnonymous. Fails loudly at startup, not silently at the first real request. - Runner updated to set AllowAnonymous=true in the two legitimate anonymous paths: 1. --no-auth flag (operator explicit opt-in) 2. No auth: block AND no --auth-url AND no channels (legacy local dev default — preserved for backward compat) Backward compat: - Existing deployments with a configured chain — unchanged. - Deployments with --no-auth — unchanged (runner sets the flag). - Deployments that ran anonymous by accident — now panic at startup with a clear, actionable message naming the flag to set. This IS a behavior change, by design. Tests (all -race clean): TestMiddleware_NilChainPanicsWithoutAllowAnonymous Asserts the panic happens AND the message mentions AllowAnonymous so operators know how to fix it. TestMiddleware_NilChainWithAllowAnonymousPassesThrough Counterpart: explicit opt-in works. TestMiddleware_NonNilChainIgnoresAllowAnonymous AllowAnonymous is only consulted when Chain is nil; a configured chain always enforces auth. The existing PR1 "nil chain passes through" test updated to use the new AllowAnonymous flag. The E2E TestE2E_NoAuthConfigured_AnonymousAccess test driver sets AllowAnonymous=true automatically when given a nil chain. Verification: go test -race ./forge-core/auth/... ./forge-cli/runtime/ — all green full sweep — 47/47 packages pass golangci-lint v2.10.1 — 0 issues
naveen-kurra
pushed a commit
to naveen-kurra/forge
that referenced
this pull request
May 23, 2026
…initializ#4) Reviewer flagged: a user who configures forge.yaml with 'auth: { required: true, providers: [...] }' and then runs the agent with --no-auth gets anonymous access silently. The flag and the YAML block were treated independently. The --no-auth happy-path short-circuit returned an anonymous middleware before consulting cfg.Auth at all. This is the second-highest-impact misconfiguration class in the auth subsystem (open prod endpoint by mistake), and it's worse than the nil-Chain case (initializ#3) because here the operator deliberately wrote 'required: true' into their config. Fix (graduated severity): auth.required == true + --no-auth → startup error (refuse) providers populated + --no-auth → warning (operator intent unclear; we proceed but make the override visible) empty auth block + --no-auth → unchanged (anonymous, no log) Error message names what's wrong AND lists the three ways to fix it: - remove --no-auth - set 'auth.required: false' - delete the 'auth:' block Warning includes provider count so dashboards can alert. Tests (all -race clean): TestResolveAuth_NoAuthWithRequiredFails auth.required=true + --no-auth → error; message contains "--no-auth conflicts with". TestResolveAuth_NoAuthWithProvidersWarns providers set, required=false + --no-auth → no error, warning emitted containing "--no-auth overrides". TestResolveAuth_NoAuthWithEmptyYAMLConfig_NoWarning Regression: --no-auth alone (no auth: block) still works without spurious warning. TestResolveAuth_NoAuthWithRequiredFalseAndProviders_WarnsNotFails Explicit Required: false is respected — warn, don't refuse. New recordLogger test helper captures Warn/Info calls so assertions can check what the runner emitted; mirrors the no-op nopLogger pattern already in use. Backward compat: - --no-auth alone (no auth: block): unchanged - --no-auth + providers + no required: warns, still anonymous - --no-auth + required: true: BREAKING — refuses to start. This is intentional; the previous behavior silently exposed a "required auth" deployment as anonymous, which the user explicitly tried to prevent in their forge.yaml. Verification: go test -race ./forge-core/auth/... ./forge-cli/runtime/ — green full sweep — 47/47 packages pass golangci-lint v2.10.1 — 0 issues
16 tasks
naveen-kurra
pushed a commit
to naveen-kurra/forge
that referenced
this pull request
May 24, 2026
Batch-clearing the "don't block merge" follow-ups from review of PR initializ#79. initializ#1 gcp_iap classifyJWTErr — use jwt v5 sentinels via errors.Is rather than substring matching (library wording shifts across patches; sentinels are public API). Special-case ErrTokenSignatureInvalid to split alg-confusion (→ ErrInvalidToken) from real bad-signature (→ ErrTokenRejected) because golang-jwt wraps both under that one sentinel. Three internal keyFunc message-matches retained — those are strings WE control, not the library's. initializ#2 gcp_iap JWKS merge-on-success — switched j.keys = newKeys to a per-kid merge. A partial-but-valid JWKS response (e.g. one kid accidentally omitted by GCP during rotation) no longer drops kids the stale-grace contract assumes we still have. Worst case is keeping a retired kid in cache; verification still fails naturally for any token signed with the retired private key. initializ#3 azure_ad GraphCache defensive copies — Get returns append([]string(nil), e.groups...) and Put stores a copy of its input. Caller mutating Identity.Groups (the auth.Identity layer treats it as freely mutable) can't poison the cache. initializ#4 forge-cli needsYAMLQuoting numeric edge cases — quote anything that resembles a YAML number (hex 0x, octal 0o, binary 0b, leading-zero "010", scientific 1e10, decimal float 3.14, signed ±N, .inf / .nan in either case). Auth-setting values rarely hit these shapes but the docstring promised "false negatives are bugs" and the Web UI POST path can supply arbitrary strings. Added looksNumeric() helper with separate allHexDigits / allOctalDigits / allBinaryDigits gates. initializ#5 aws_sigv4 identity_cache_test — replaced string(rune(i)) with strconv.Itoa(i). Surrogate code points (0xD800..0xDFFF) all map to U+FFFD, so the eviction-threshold test was silently building ~10 distinct keys instead of 10_001 and the sweep never ran. initializ#6 http.NewRequestWithContext error handling — fixed the two `req, _ := ...` antipatterns in gcp_iap/iap_jwks.go and azure_ad/graph_client.go. Hardcoded URLs make the failure currently unreachable, but errcheck-clean is the discipline. initializ#7 gcp_iap HS256-with-EC-public-key alg-confusion test — pinned the most dangerous attack shape: attacker fetches the verifier's public key from JWKS (open by design), uses raw X/Y bytes as the HMAC "secret", signs an HS256 token. A non-whitelisting verifier would HMAC-verify it. Our keyFunc rejects on alg != "ES256" BEFORE key lookup; this test confirms. Tests added: TestGraphCache_GetReturnsDefensiveCopy, TestGraphCache_PutStoresDefensiveCopy, TestProvider_HS256WithECPublicKeyAsSecret_Rejected. Existing TestProvider_RS256Token_Rejected still passes (alg-confusion still classified as ErrInvalidToken under the new sentinel-based path). 42 packages green, lint + gofmt clean.
initializ-mk
pushed a commit
that referenced
this pull request
May 24, 2026
Batch-clearing the "don't block merge" follow-ups from review of PR #79. #1 gcp_iap classifyJWTErr — use jwt v5 sentinels via errors.Is rather than substring matching (library wording shifts across patches; sentinels are public API). Special-case ErrTokenSignatureInvalid to split alg-confusion (→ ErrInvalidToken) from real bad-signature (→ ErrTokenRejected) because golang-jwt wraps both under that one sentinel. Three internal keyFunc message-matches retained — those are strings WE control, not the library's. #2 gcp_iap JWKS merge-on-success — switched j.keys = newKeys to a per-kid merge. A partial-but-valid JWKS response (e.g. one kid accidentally omitted by GCP during rotation) no longer drops kids the stale-grace contract assumes we still have. Worst case is keeping a retired kid in cache; verification still fails naturally for any token signed with the retired private key. #3 azure_ad GraphCache defensive copies — Get returns append([]string(nil), e.groups...) and Put stores a copy of its input. Caller mutating Identity.Groups (the auth.Identity layer treats it as freely mutable) can't poison the cache. #4 forge-cli needsYAMLQuoting numeric edge cases — quote anything that resembles a YAML number (hex 0x, octal 0o, binary 0b, leading-zero "010", scientific 1e10, decimal float 3.14, signed ±N, .inf / .nan in either case). Auth-setting values rarely hit these shapes but the docstring promised "false negatives are bugs" and the Web UI POST path can supply arbitrary strings. Added looksNumeric() helper with separate allHexDigits / allOctalDigits / allBinaryDigits gates. #5 aws_sigv4 identity_cache_test — replaced string(rune(i)) with strconv.Itoa(i). Surrogate code points (0xD800..0xDFFF) all map to U+FFFD, so the eviction-threshold test was silently building ~10 distinct keys instead of 10_001 and the sweep never ran. #6 http.NewRequestWithContext error handling — fixed the two `req, _ := ...` antipatterns in gcp_iap/iap_jwks.go and azure_ad/graph_client.go. Hardcoded URLs make the failure currently unreachable, but errcheck-clean is the discipline. #7 gcp_iap HS256-with-EC-public-key alg-confusion test — pinned the most dangerous attack shape: attacker fetches the verifier's public key from JWKS (open by design), uses raw X/Y bytes as the HMAC "secret", signs an HS256 token. A non-whitelisting verifier would HMAC-verify it. Our keyFunc rejects on alg != "ES256" BEFORE key lookup; this test confirms. Tests added: TestGraphCache_GetReturnsDefensiveCopy, TestGraphCache_PutStoresDefensiveCopy, TestProvider_HS256WithECPublicKeyAsSecret_Rejected. Existing TestProvider_RS256Token_Rejected still passes (alg-confusion still classified as ErrInvalidToken under the new sentinel-based path). 42 packages green, lint + gofmt clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
web_searchtool into a provider pattern with pluggable backends (Tavily and Perplexity). Auto-detects from env vars (TAVILY_API_KEY>PERPLEXITY_API_KEY), with explicit override viaWEB_SEARCH_PROVIDER.tavily-searchto the skill registry with an embedded shell script (scripts/tavily-search.sh), skill markdown, andforge skills addCLI command for vendoring skills into projects.web_searchis selected, the wizard now presents a provider choice (Tavily recommended / Perplexity) before prompting for the API key.sendChatAction("typing")immediately when a message is received and repeats every 4s while the LLM processes, for both polling and webhook modes.New files
forge-core/tools/builtins/web_search_provider.gowebSearchOptsforge-core/tools/builtins/web_search_tavily.goforge-core/tools/builtins/web_search_perplexity.goweb_search.go)forge-core/registry/skills/tavily-search.mdforge-core/registry/scripts/tavily-search.shTest plan
go test ./forge-core/tools/builtins/— 16/16 pass (incl. Tavily provider, Perplexity provider, override, auto-detect, no-key)go test ./forge-core/registry/— 7/7 pass (incl. tavily-search index, script loading)go test ./forge-core/security/— 22/22 passgo test ./forge-plugins/channels/telegram/— 13/13 pass (incl. sendChatAction, typing indicator)go build ./forge-core/...andgo build ./forge-cli/...compile cleanly